-
-
Notifications
You must be signed in to change notification settings - Fork 115
Synchronization of transports #7485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1e4a729 to
3b2f96a
Compare
7baac2d to
7c28f2c
Compare
7c28f2c to
fb9a033
Compare
759f4ba to
e6a2d94
Compare
fb9a033 to
aa10a3d
Compare
e6a2d94 to
4fa2153
Compare
aa10a3d to
7ec3e47
Compare
I don't remember where exactly, but it was discussed that for the UI code it's easier to do everything on events rather then to handle also UI-triggered config changes. But if this event is only for tests, that doesn't matter probably. |
7ec3e47 to
b5fea77
Compare
not really, ex. if user has the transports list open to check transport is synchronized, and adds a transport in another device, the list will not be refreshed, user will need to close and open again for the list to refresh, a minor tho btw, I guess changing/marking the main transport should also be synchronized, is that the case? (sorry for not trying to dive into the rust code to figure out) |
I also think refreshing the list while you are editing it is going to cause more troubles than be useful. I don't expect users to edit transports from two devices simultaneously. Will mark the event clearly as an event that is for testing only.
Yes, it is synchronized. Not explicitly, but just when you receive a sync message, the address is changed to the one that sync message comes from. The test checks that this works. |
b5fea77 to
38eac74
Compare
4f73fb5 to
8f3415b
Compare
8f3415b to
6fcd396
Compare
| ON CONFLICT (addr) | ||
| DO UPDATE SET entered_param=excluded.entered_param, | ||
| configured_param=excluded.configured_param, | ||
| add_timestamp=excluded.add_timestamp", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this seems to be correct for transports synchronization, but when the user adds a transport, shouldn't add_timestamp be greater than remove_timestamp if it exists for the transport? It should be always so though, if the device clocks are synchronized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If added timestamp is in the past, then next sync will remove it back. This can be improved, also removed_transports need to expire eventually, I added TODO item to multi-transport for this.
This should be rare that this cause problems, first of all user need to remove and resetup the transport with the same address so it cannot happen with chatmail and then either removal timestamp needs to be from the future (very unlikely that clock jumps to the future) or addition timestamp needs to be in the past (which may be if the clock is reset and does not get synchronized, but still very uncommon).
this is not even a worsening, as it is like that forever |
The last commits adds actual transport synchronization, commits before split into PR #7530 are refactorings and preparation.
New
TransportsModifiedevent is meant for tests, it is currently emitted when sync message is received and not when the user changes the transports manually as it is already known to the UI.Transport with ID 1 is never synced except for deletion to avoid ever sending the credentials of the first transport to other email servers. The only downside is that if the password is changed, the user will have to change it on all devices, but this avoids all the discussion of reducing security compared to the current state. The first transport with multi-transport is handled he same way as before without multi-transport.